🟡 Taste Rating: Acceptable

Pragmatic fix to a real problem, but breaks existing behavior without proper user migration guidance.

The core change is simple and solves the claude-4.6 incompatibility issue. However, this is a significant behavior change that moves from deterministic outputs (temp=0.0) to provider-dependent defaults. See inline comments for specific concerns.

VERDICT: ✅ Worth merging after addressing migration documentation

KEY INSIGHT: This is the right technical fix, but users who relied on deterministic behavior need explicit guidance on setting temperature=0.0 in their configs.

Thank you for this! I think maybe we can then remove the DEFAULT_TEMPERATURE_MODELS too? That’s there because Kimi is a reasoning model that doesn’t accept effort, if I recall correctly, but it needs temperature 1 like reasoning models.

I had the agent test Kimi:

• for 2.5, it worked fine with None
• for 2-thinking, no deployments found. (maybe it’s legacy now; we may want to remove it from eval proxy, since it doesn’t appear to work anymore)

Please see PR:

• #1994

🟡 Taste Rating: Acceptable - Solves a real problem with elegant simplification, but introduces a breaking change that needs careful consideration.

Verdict: Worth merging with minor improvements to address backward compatibility concerns and design consistency.

Key Insight: This trades deterministic defaults for provider flexibility—a pragmatic choice, but one that changes behavior for all existing users.

🔴 Taste Rating: Needs Rework

This PR simplifies the code (good!) but introduces a breaking change that violates the "never break userspace" principle. Users relying on deterministic outputs (temperature=0.0) will now get unpredictable provider defaults.

[CRITICAL ISSUES] - Must Fix

1. Breaking Change Without Documentation

File: openhands-sdk/openhands/sdk/llm/llm.py (lines 180-185)

🔴 Critical: This description doesn't mention that this is a BREAKING CHANGE. Users who relied on temperature=0.0 for deterministic outputs will now get provider-specific defaults (which vary: OpenAI defaults to 1.0, Anthropic to 1.0, etc.).

You've broken userspace without documenting it. At minimum:

1. Clear migration guide: "To preserve deterministic behavior, explicitly set temperature=0.0"
2. Deprecation warning in release notes
3. Consider a compatibility flag for one release cycle

Suggested fix:

temperature: float | None = Field(
    default=None,
    ge=0,
    description=(
        "Sampling temperature for response generation. "
        "Defaults to None (uses provider default, typically 1.0 for most models). "
        "BREAKING CHANGE: Previously defaulted to 0.0 for deterministic outputs. "
        "To restore deterministic behavior, explicitly set temperature=0.0. "
        "Higher values (0.7-1.0) increase creativity and randomness."
    ),
)

2. Unused Parameter - Code Smell

File: openhands-sdk/openhands/sdk/llm/utils/model_features.py (line 183)

🔴 Critical: The _model parameter is now completely ignored (indicated by the underscore prefix). This is a code smell that screams "delete this function entirely."

If the model parameter is never used, why does this function exist? Either:

1. Make it a module-level constant: DEFAULT_TEMPERATURE = None
2. Delete it and use None directly where needed
3. Or explain why we need the indirection

The function signature promises model-specific logic but delivers nothing. That's broken by design.

[IMPROVEMENT OPPORTUNITIES] - Should Fix

3. Misleading Documentation

File: openhands-sdk/openhands/sdk/llm/utils/model_features.py (lines 184-189)

🟠 Important: The docstring says "Return the default temperature for a given model pattern" but the model is completely ignored. This is a lie.

Suggested fix:

def get_default_temperature(_model: str) -> float | None:
    """Return None to allow providers to use their default temperature.
    
    This function exists for backward compatibility and may be removed in future versions.
    To use provider defaults, pass temperature=None when creating LLM instances.
    """
    return None

4. Missing Integration Tests

File: tests/sdk/llm/test_model_features.py (lines 342-346)

🟠 Important: These tests only verify the function returns None. They don't test what matters:

1. Does passing temperature=None to a provider actually work?
2. Does it fix the claude-4.6 issue mentioned in #1913?
3. What temperature does each provider actually use when None is passed?

You're testing the implementation, not the behavior. Add tests that:

• Verify LiteLLM/providers accept temperature=None
• Confirm claude-4.6 no longer errors
• Document what "provider default" means for major providers

VERDICT

❌ Needs rework: The simplification is good in principle, but the breaking change and unused parameter issues must be addressed first.

KEY INSIGHT

Simplifying code by removing special cases is "good taste" - BUT you cannot break existing user expectations in the process. Either preserve backward compatibility with explicit opt-in to new behavior, or clearly document the breaking change and provide a migration path.